fix: inherit run formatting when inserting inline structured content (SD-1421)#2501
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7747620487
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
packages/super-editor/src/extensions/structured-content/structured-content-commands.js
Outdated
Show resolved
Hide resolved
packages/super-editor/src/extensions/structured-content/structured-content-commands.js
Outdated
Show resolved
Hide resolved
…ne SDT - Fall back to plain replaceWith when the selection spans multiple runs, instead of only replacing the first run and leaving the rest untouched - Pass state.storedMarks to getFormattingStateAtPos so pending toolbar formatting (e.g. toggling bold before inserting) is respected
caiopizzol
left a comment
There was a problem hiding this comment.
@luccas-harbour nice work — the run splitting and font inheritance logic looks good. both points from the earlier automated review are handled.
two things: the cursor lands in the wrong spot after inserting a field mid-run (line 210), and inserting at the very start of a run isn't tested yet. left inline comments with details. neither blocks the fix for the reported bug, but the cursor one could bite in the template builder.
| // surrounding text. The run-split logic below prevents an outer run | ||
| // from wrapping the SDT itself. | ||
| const runType = schema.nodes.run; | ||
| if (runType && !options.json && content.isText) { |
There was a problem hiding this comment.
when options.json is used, this whole block is skipped — the inserted field won't pick up the surrounding font. that's probably intentional, but a comment here would save the next person from wondering if it's a bug.
| fragments.push(runType.create(parentRun.attrs, rightContent, parentRun.marks)); | ||
| } | ||
|
|
||
| tr.replaceWith(runStart, runEnd, fragments); |
There was a problem hiding this comment.
after this replace, the cursor ends up at the end of the text instead of right after the new field. users won't notice if it's triggered by a button, but if they keep typing after inserting a field in the template builder, text goes to the wrong spot. worth fixing with a tr.setSelection(...) after the replace?
| @@ -556,6 +556,122 @@ describe('updateStructuredContentByGroup', () => { | |||
| }); | |||
There was a problem hiding this comment.
missing a test for when the cursor is at the very start of a run — that's the case where there's no text on the left side of the split. worth adding to make sure no empty run sneaks into the output.
Summary
insertStructuredContentInlineproducedrun > structuredContent > run > textnesting — the SDT is now placed at paragraph level between split sibling runsProblem
insertStructuredContentInlineused a baretr.replaceWith(from, to, sdtNode)to insert the SDT. When the cursor was inside a run, ProseMirror placed the SDT as a child of the run (sincerunhascontent: 'inline*'). ThenwrapTextInRunsPluginnormalized this intorun{null} > SDT > run > text, creating unwanted nesting with an empty outer run.Additionally, the SDT's text content was created without any marks or run wrapping, so it appeared unformatted regardless of the surrounding text style.
Solution
Two changes to
insertStructuredContentInline:runPropertiesfrom the cursor position viagetFormattingStateAtPosand wraps the text in a properly formatted run inside the SDT[leftRun, sdtNode, rightRun]at the paragraph level, producing the correct flat structure